Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: modernize JS and tighten equality checking in test-child-process-detached #8580

Closed
wants to merge 1 commit into from
Closed

test: modernize JS and tighten equality checking in test-child-process-detached #8580

wants to merge 1 commit into from

Conversation

wzoom
Copy link
Contributor

@wzoom wzoom commented Sep 17, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Cleanup according to nodejs/code-and-learn#56

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016
sejoker
sejoker approved these changes Sep 17, 2016
View reviewed changes
Copy link
Contributor

@sejoker sejoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eljefedelrodeodeljefe
Copy link
Contributor

Please revisit commit message to match contribution guidelines. Also what refers flankyness to?

Otherwise LGTM

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Sep 17, 2016
@wzoom
Copy link
Contributor Author

wzoom commented Sep 17, 2016

Revisited the commit mesage.

@eljefedelrodeodeljefe
Copy link
Contributor

Maybe we could get @addaleax or @thealphanerd or @jasnell to give a LGTM and I merge it.

@eljefedelrodeodeljefe
Copy link
Contributor

@wzoom Also just a tip for future contributions, as you will get more familiar with git foo: you could have run git commit --amend (make your commit change) and git push --force to your fork to stick with one commit. I am gonna fix this while landing anyhow since I need to edit the commit for landing itself.

@wzoom
Copy link
Contributor Author

wzoom commented Sep 17, 2016

Thanks. I did just that (ammend), but somehow git forced me to merge afterwards :/

cjihrig
cjihrig approved these changes Sep 17, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I don't see how this addresses any flakiness though. Also, would you care to take a crack at squashing the commits.

@wzoom wzoom changed the title test: fix test-child-process-detached flankyness test: modernize JS and tighten equality checking in test-child-process-detached Sep 17, 2016
@wzoom
Copy link
Contributor Author

wzoom commented Sep 17, 2016

Removed the useless commits, now it should be ok. Flankyness was a mistake.

addaleax
addaleax requested changes Sep 17, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one nit addressed

test/parallel/test-child-process-detached.js
var spawn = require('child_process').spawn;
var childPath = path.join(common.fixturesDir,
const spawn = require('child_process').spawn;
const childPath = path.join(common.fixturesDir,
'parent-process-nonpersistent.js');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can you indent this line 2 characters so that the arguments to path.join are still aligned?

@wzoom
test: modernize JS and tighten equality checking
617be60 
Changed var --> const and let.
Changed assert.equal !== --> assert.notStrictEqual
Correctly aligned argument
@wzoom
Copy link
Contributor Author

wzoom commented Sep 19, 2016

FYI @addaleax Fixed.

lpinca
lpinca approved these changes Sep 19, 2016
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

addaleax
addaleax approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, landing…

@addaleax
Copy link
Member

Okay, I’m not going to land this without a proper CI run, so, CI: https://ci.nodejs.org/job/node-test-commit/5175/

@addaleax
Copy link
Member

Landed in d59074c! Thanks for your contribution! 🎉

@addaleax addaleax closed this Sep 20, 2016
addaleax pushed a commit that referenced this pull request Sep 20, 2016
@wzoom @addaleax
test: modernize JS and tighten equality checking
d59074c 
Changed var --> const and let.
Changed assert.equal !== --> assert.notStrictEqual
Correctly aligned argument

PR-URL: #8580
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@wzoom @Fishrock123
test: modernize JS and tighten equality checking
fc6b497 
Changed var --> const and let.
Changed assert.equal !== --> assert.notStrictEqual
Correctly aligned argument

PR-URL: #8580
Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sejoker sejoker sejoker approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@wzoom @eljefedelrodeodeljefe @addaleax @sejoker @lpinca @cjihrig @mscdex @MylesBorins @nodejs-github-bot